Bound batch decode by item limit (PLT-820) by amir-deris · Pull Request #98 · sei-protocol/go-ethereum

amir-deris · 2026-09-04T10:38:58Z

Summary

parseMessage decoded every batch element before handleBatch checked the item limit, so a compact array of scalars could allocate hundreds of MiB per request. The limit is now enforced inside parseMessage (the single decode path for HTTP and WS), stopping one element past the limit.

The overshoot is deliberate and load-bearing: handleBatch rejects on len(msgs) > h.batchRequestLimit, so returning exactly itemLimit elements would make an over-limit batch look in-limit and be executed. parseMessage's break and that comparison must stay in agreement; both directions are now pinned by tests (see below).

Behavior change: null error id for notification-front-loaded batches

Because only the first itemLimit+1 elements are decoded, a call that appears after that prefix can no longer contribute its id to the batch too large error. Such a batch is now answered with a null id where it previously carried the id of the first call anywhere in the array.

This is user-visible, and for a client it is a hang rather than a cosmetic difference: a null id matches nothing in h.respWait (rpc/handler.go:551), so the response is logged as "Unsolicited RPC response" and the caller's requestOp blocks until its context deadline instead of getting an immediate batch too large error.

Impact in practice is narrow. Geth's own BatchCallContext assigns an id to every element, so in-tree callers cannot hit it; it requires a third-party client that front-loads more than itemLimit notifications ahead of its first call. Accepted as the cost of bounding the allocation, and pinned by a case in testdata/invalid-batch-toolarge.js rather than left to a code comment.

WithBatchItemLimit's doc comment is corrected while here — it claimed the option did not affect batches on the client side, which was already inaccurate (handleBatch applies the limit to inbound response batches too) and is more misleading now that the limit also truncates their decode.

Test plan

Unit tests for limit behavior and allocation bounds
HTTP and WS oversize-batch rejection + connection survival
Truncation is proven end-to-end, not just at the parseMessage level: makeTruncationProbe sends a batch whose only call sits just past the decoded prefix, so a null error id is positive evidence that the codec passed the limit into parseMessage. The batch too large rejection alone proves nothing — the handler already produced it before this change.
Mutation-checked. Each of these fails at least one test:
- dropping attachHandler from serveSingleRequest -> HTTP probe reports id 99
- parseMessage breaking at the limit instead of one past -> invalid-batch-toolarge.js sees the batch executed (the silent-execution failure mode)
- handleBatch tightened to >= -> reqresp-batch.js sees an exactly-at-limit batch rejected

Known gap (pre-existing, not addressed here)

attachHandler wires the codec through an assertion on the unexported handlerSetter interface, so an external ServerCodec decorator that forwards readBatch without promoting setHandler leaves batchItemLimit() at 0 and decodes unbounded. This gap is identical in attachBudgetHandler before this PR, nothing in-tree wraps ServerCodec (both built-in codecs are covered — websocketCodec embeds *jsonCodec), and the handler-level check still rejects the batch, so the exposure degrades to "allocate, then reject" for external embedders. Worth plumbing the limit through codec construction as a follow-up now that bounded decoding is a security property rather than a budgeting optimization.

parseMessage decoded every element of a batch array before the item limit was checked in handleBatch, so a compact 10 MiB array of scalars expanded into one jsonrpcMessage per element and could lead to hundreds of MiB of heap per request. Push the limit into parseMessage, the one function both read paths pass through, and stop the decode one element past it. The handleBatch count check and the WS aggregate byte admission remain as defense in depth.

cursor · 2026-09-04T10:41:33Z

PR Summary

Medium Risk
Touches the shared JSON-RPC decode path for HTTP, WebSocket, and IPC; behavior is mostly unchanged for valid batches but error id selection can differ when decode is truncated.

Overview
Fixes a DoS vector where oversized JSON-RPC batch arrays could force per-element allocations during decode before the existing batch item limit was checked.

parseMessage now takes an itemLimit and stops decoding after limit+1 batch elements (one extra so handleBatch’s len(msgs) > limit rejection still works). JSON and WebSocket codecs pass the handler’s limit via renamed attachHandler / setHandler wiring; serveSingleRequest now attaches the handler on HTTP so that path is covered too.

Observable behavior: “batch too large” errors may use a null id when the only call sits past the decoded prefix; batches of exactly the limit are still served. Docs for WithBatchItemLimit are clarified.

Adds unit, allocation, HTTP/WS integration tests and testdata for truncation and at-limit batches.

^{Reviewed by Cursor Bugbot for commit 4634ec8. Bugbot is set up for automated code reviews on this repo. Configure here.}

seidroid

Sound, well-scoped fix: pushing the batch item limit into parseMessage genuinely bounds the per-element jsonrpcMessage allocation on both the WS and HTTP read paths, and the deliberate "decode one past the limit" overshoot correctly preserves handleBatch's len(msgs) > limit rejection. No blockers; the notes below are a documented behavior change to the batch-too-large error id, the pre-existing decorated-codec gap Codex flagged, and some test-coverage gaps.

Findings: 0 blocking | 8 non-blocking | 4 posted inline

Blockers

None at the file/PR level.

Non-blocking

The Cursor review pass (cursor-review.md) produced no output — that file is empty, so this synthesis merges only my own findings with Codex's single finding. REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied.
Test-coverage gap: neither TestHTTPOversizeBatchRejected nor TestWSOversizeBatchRejectedAndConnectionSurvives actually proves the fix. Both assert the batch too large rejection, which the code already produced before this change (via handleBatch). The only real regression guard is TestParseMessageBatchAllocationsBounded, which calls parseMessage directly and so does not cover the codec wiring. Consider an end-to-end assertion — e.g. assert codec.(*jsonCodec).batchItemLimit() != 0 after serveSingleRequest wires the handler, or wrap the HTTP/WS assertions in testing.AllocsPerRun. Without that, a future change that drops attachHandler from serveSingleRequest (server.go:226) would silently reintroduce the unbounded decode with every test still green.
No test covers the changed respondWithBatchTooLarge id behavior (a batch whose first itemLimit+1 decoded elements are all notifications). Worth a table entry so the tradeoff is pinned down rather than only described in a comment.
WithBatchItemLimit's doc comment (rpc/client_opt.go:130-133) says the option "does not affect batch requests sent by the client." That was already inaccurate — handleBatch applied batchRequestLimit to inbound batch responses too — and this PR makes the coupling tighter by also truncating the decode of those responses. Behavior is unchanged (both before and after, an over-limit response batch is rejected rather than dispatched), so this is not a regression, but the doc comment is now more misleading and is worth correcting while you are here.
4 suggestion(s)/nit(s) flagged inline on specific lines.

seidroid · 2026-09-04T10:48:15Z

+		// what handleBatch's own count check reads to reject the batch, and decoding
+		// past it would allocate a jsonrpcMessage per element of an array the server
+		// has already decided not to serve.
+		if itemLimit > 0 && len(msgs) > itemLimit {


[suggestion] The > itemLimit (rather than >= itemLimit) overshoot is correct and necessary — it is what keeps handleBatch's len(msgs) > h.batchRequestLimit check (handler.go:317) firing — but the correctness of this line now depends on a strict-> comparison in a different file. If anyone ever tightens handler.go:317 to >=, an over-limit batch would be silently truncated to itemLimit elements and executed rather than rejected, which is a far worse failure than the one being fixed here.

The comment explains the why but doesn't guard it. Two cheap options: reference handler.go's check explicitly by name in the comment, or make the contract explicit by having parseMessage return a truncated bool that handleBatch treats as an unconditional reject. Either removes the silent-execution failure mode.

(The limit source itself is safe by construction — jsonCodec.batchItemLimit() reads c.handler.batchRequestLimit from the very same handler that performs the check — so the two values can never diverge. It is only the comparison operator that is load-bearing across files.)

seidroid · 2026-09-04T10:48:15Z

 	// This is the best we can do, given that the protocol doesn't have a way
-	// of reporting an error for the entire batch.
+	// of reporting an error for the entire batch. The batch is only decoded up to
+	// the item limit, so a batch whose every decoded element is a notification is


[suggestion] Worth calling out in the PR description, not just this comment: this is a user-visible protocol regression, and the consequence is a client hang rather than a cosmetic id change.

For a batch whose first itemLimit+1 elements are all notifications and whose later elements include a call, the error response now carries a null id. On the receiving side, handleResponses matches by h.respWait[string(msg.ID)] (handler.go:551); a null id matches nothing, so the response is logged as "Unsolicited RPC response" and the caller's requestOp blocks until its context deadline instead of getting an immediate batch too large error.

Impact is limited in practice — geth's own BatchCallContext assigns an id to every element, so it can't hit this — but a third-party client that front-loads notifications can. The tradeoff looks acceptable given the memory bound it buys; please just make it explicit in the description/changelog rather than only in a code comment, and consider a table case in TestParseMessageBatchItemLimit pinning the behavior.

seidroid · 2026-09-04T10:48:15Z

+// attachHandler wires h into codec so reads on it observe the handler's limits. A codec
+// that does not implement handlerSetter reads unlimited, and the handler's own checks
+// remain the backstop.
+func attachHandler(codec ServerCodec, h *handler) {


[suggestion] Raised by Codex, kept with a lower severity. attachHandler's type assertion only matches codecs implementing the unexported handlerSetter, so a ServerCodec decorator that forwards readBatch without promoting setHandler leaves the wrapped jsonCodec.handler nil, batchItemLimit() returns 0, and the full batch is decoded and allocated before handleBatch rejects it.

Two things temper this relative to Codex's "Medium":

It is pre-existing, not introduced here — attachBudgetHandler had exactly the same gap, and the doc comment you added on this function states the fallback explicitly.

Nothing in-tree wraps ServerCodec; the only decorator is the passthroughCodec local type in ws_admission_test.go. Both built-in codecs are covered (websocketCodec embeds *jsonCodec, so setHandler is promoted).

So the exposure is external embedders only, and the handler-level check still rejects the batch — the DoS is degraded to "allocation happens, then rejection," not "batch executes." Still, since bounded decoding is now a security property rather than just a budgeting optimization, it's worth making it not depend on a private-interface assertion — e.g. plumb the item limit through codec construction, or fall back to a package-level default in parseMessage when no handler is wired.

seidroid · 2026-09-04T10:48:15Z

+	httpsrv := httptest.NewServer(srv)
+	defer httpsrv.Close()
+
+	resp, err := http.Post(httpsrv.URL, "application/json", strings.NewReader(makeCallBatch(50000)))


[nit] makeCallBatch(50000) builds a ~3.3MB request body, uncomfortably close to defaultBodyLimit (5MiB, rpc/http.go:36) — if the element template ever grows, this test starts failing with a 413 for reasons unrelated to what it's asserting. It also costs 50k fmt.Sprintf calls plus the join on every run.

Since the server only ever decodes itemLimit+1 = 5 elements, a few hundred elements would exercise the identical path. Suggest dropping the count substantially, or adding a comment noting the deliberate margin against the body limit.

masih · 2026-09-04T11:11:01Z

+
+// TestHTTPOversizeBatchRejected covers the single-request path, which builds its handler
+// separately from ServeCodec and so wires the codec up on its own.
+func TestHTTPOversizeBatchRejected(t *testing.T) {


Assert the codec actually observed the limit? Same for TestWSOversizeBatchRejectedAndConnectionSurvive test.

masih · 2026-09-04T11:12:13Z

+// TestParseMessageBatchAllocationsBounded is the regression guard for the report: a
+// compact array of scalars must not allocate a jsonrpcMessage per element before the
+// item limit is consulted.
+// It cannot run in parallel: testing.AllocsPerRun panics in a parallel test.


Maybe time to upgrade the go module?

Actually upgrading go.mod does not remove the need to keep this test non-parallel.
If this test calls t.Parallel():

in go version 1.23 / 1.24 => No panic — results can be flaky

1.25+ => AllocsPerRun can panic if other parallel tests in the package are actively running — and rpc package has many t.Parallel() tests.

Review feedback: the HTTP and WS tests asserted only the "batch too large" rejection, which handleBatch already produced before the item limit reached parseMessage, so neither covered the codec wiring. Both now send a probe batch whose only call sits just past the decoded prefix. respondWithBatchTooLarge reports the first decoded call's id, so a null id is positive evidence the codec observed the limit; a paired case with a leading call still expects its id, so the null id reads as truncation rather than ids going missing. Pin the cross-file contract the overshoot depends on: parseMessage must return itemLimit+1 elements and handleBatch must reject on a strict '>'. Neither side was covered, so tightening either would have gone unnoticed. Add a reqresp-batch.js case for a batch of exactly the limit, an invalid-batch-toolarge.js case for the null-id behavior, and name both in the parseMessage comment. Also shrink makeCallBatch's element count, which built a ~3.3MB body against a 5MiB defaultBodyLimit for a path that only decodes itemLimit+1 elements, and correct WithBatchItemLimit's doc comment: the limit applies to inbound batches, including batched responses to the client's own requests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

The fix is sound: parseMessage now stops one element past the item limit, which correctly preserves handleBatch's strict len(msgs) > limit rejection while bounding per-element allocations, and it's wired into every built-in codec path (WS, IPC/stdio via ServeCodec, and HTTP via the new attachHandler in serveSingleRequest). No blocking issues found; a few non-blocking notes on latent budget coupling on the HTTP path, allocation-test robustness, and the unexported setHandler extension point.

Findings: 0 blocking | 7 non-blocking | 4 posted inline

Blockers

None at the file/PR level.

Non-blocking

Observable protocol change: the batch too large error can now carry a null id where it previously carried the id of a call appearing later in the batch (only when the first itemLimit+1 elements are all notifications). This is documented in handler.go and pinned in testdata/invalid-batch-toolarge.js, and the common all-calls batch is unaffected, but it is a divergence from upstream go-ethereum worth calling out in the PR description / release notes.
The Cursor second-opinion pass produced no output (cursor-review.md is empty), so this review merges only my findings with Codex's. REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied.
Coverage note: the new tests cover WS and HTTP, but there is no direct test that the IPC/stdio ServeCodec path (plain NewCodec) is bounded. It is covered transitively by TestParseMessageBatchItemLimit plus the attachHandler wiring, so this is optional.
4 suggestion(s)/nit(s) flagged inline on specific lines.

seidroid · 2026-09-04T14:33:36Z


 	h := newHandler(ctx, codec, s.idgen, &s.services, s.batchItemLimit, s.batchResponseLimit, nil, s.readLimit, nil, s.wsAdmissionTimeout)
 	h.allowSubscribe = false
+	attachHandler(codec, h)


[suggestion] Attaching the handler here also switches on the other behavior c.handler != nil gates in jsonCodec.readBatch: the acquirePreDecode call. Today that is inert because serveSingleRequest passes a nil wsConcurrentBudget (acquirePreDecode returns early without setting preDecodeHeld), so this is safe as written.

It is a latent trap though: nothing on the HTTP single-request path ever calls commitFrameBudget or releasePreDecode after a successful read, so the moment a non-nil budget is passed to this newHandler call, every HTTP request would permanently leak readLimit bytes of semaphore weight. Worth a short comment here (or a defer h.releasePreDecode()) recording that the nil budget is load-bearing.

seidroid · 2026-09-04T14:33:36Z

+	)
+	raw := json.RawMessage(makeScalarBatch(elements))
+
+	allocs := testing.AllocsPerRun(2, func() {


[suggestion] testing.AllocsPerRun measures runtime.MemStats.Mallocs, which is process-wide, not per-goroutine. Package rpc leaves background goroutines alive from earlier tests (WS ping loops, httptest servers, handler call goroutines), and any allocation they make during the measurement window is attributed to parseMessage.

The 1000-alloc ceiling against an expected ~100-300 gives decent headroom, so this probably won't flake often — but when it does the failure will be confusing and unrelated to this code. Consider making it explicitly differential instead, e.g. measure parseMessage(smallBatch, itemLimit) as a baseline in the same run and assert the 200k-element case is within a small multiple, so ambient allocations cancel out.

seidroid · 2026-09-04T14:33:36Z

+// handlerSetter is implemented by codecs that need the handler wired in to enforce its
+// read limits (jsonCodec and, via embedding, websocketCodec).
+type handlerSetter interface {
+	setHandler(h *handler)


[nit] Raised by the Codex pass, and it's a fair point though I'd rank it below blocking: setHandler is unexported, so a ServerCodec decorator defined outside package rpc (embedding the result of NewCodec and forwarding readBatch) cannot satisfy handlerSetter, and those deployments keep the unbounded decode this PR is fixing.

Two things soften it: the shape is pre-existing (it was setBudgetHandler before this rename), and the handler-side len(msgs) > batchRequestLimit check still rejects the batch — only the allocation bound is lost, not the limit itself. There are also no such wrappers in this repo. The doc comment below already states the fallback accurately; the only thing I'd add is that this now silently downgrades a security bound rather than just a performance one, so an exported opt-in (or documenting ServerCodec decorators as unsupported) would be worth a follow-up.

seidroid · 2026-09-04T14:33:36Z

-// batch requests sent by the client.
+// Note: this option applies to batches the client receives: both batch requests sent by
+// the server on a bidirectional connection and batched responses to the client's own
+// requests. A batch with more items than the limit is rejected instead of dispatched, and


[nit] "rejected instead of dispatched" undersells what happens for the response case: when the over-limit batch is a batch of responses to the client's own calls, handleBatch routes to respondWithBatchTooLarge, which writes a batch too large error frame back to the peer while the pending BatchElems all fail with ErrMissingBatchResponse. Worth naming that outcome explicitly so callers know setting this option can make their own batch calls fail rather than just protecting them.

amir-deris self-assigned this Sep 4, 2026

amir-deris changed the title ~~Bound batch decoding by the item limit (PLT-820)~~ Bound batch decode by item limit (PLT-820) Sep 4, 2026

amir-deris marked this pull request as ready for review September 4, 2026 10:41

amir-deris requested review from bdchatham and masih September 4, 2026 10:41

seidroid Bot approved these changes Sep 4, 2026

View reviewed changes

sei-will approved these changes Sep 4, 2026

View reviewed changes

masih approved these changes Sep 4, 2026

View reviewed changes

seidroid Bot approved these changes Sep 4, 2026

View reviewed changes

amir-deris merged commit 244eec7 into main Sep 4, 2026
13 of 15 checks passed

amir-deris deleted the amir/plt-820-parse-message-allocation-check branch September 4, 2026 14:38

Conversation

amir-deris commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Behavior change: null error id for notification-front-loaded batches

Test plan

Known gap (pre-existing, not addressed here)

Uh oh!

cursor Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

seidroid Bot left a comment

Choose a reason for hiding this comment

Blockers

Non-blocking

Uh oh!

seidroid Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

seidroid Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

seidroid Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

seidroid Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

masih Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

masih Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

amir-deris Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

seidroid Bot left a comment

Choose a reason for hiding this comment

Blockers

Non-blocking

Uh oh!

seidroid Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

seidroid Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

seidroid Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

seidroid Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amir-deris commented Sep 4, 2026 •

edited

Loading

cursor Bot commented Sep 4, 2026 •

edited

Loading